✨ retry connecting the cloudevents client #280

skeeey · 2023-09-15T01:36:47Z

Summary

Retry connecting the cloudevents client when the client has network error

skeeey · 2023-09-15T01:46:39Z

/assign @qiujian16

skeeey · 2023-09-15T03:38:18Z

/hold

skeeey · 2023-09-20T03:12:49Z

/unhold

qiujian16 · 2023-09-20T06:53:41Z

cloudevents/generic/baseclient.go

+					// TODO enhance the cloudevents SKD to avoid wrapping the error type, then we can only handle the
+					// net connection errors here
+					klog.Errorf("failed to reconnect, %v", err)
+					<-connBackoffManager.Backoff().C()


if it goes into this branch, will it hang on the select in the next loop?

the select in the next loop need to wait the backoff, the backoff is a blocking action

qiujian16 · 2023-09-20T06:55:14Z

cloudevents/generic/baseclient.go

+				if receiverCancel != nil {
+					receiverCancel()
+				}
+				retrying = true


can we just set c.cloudEventsClient = nil here?

the cloudEventsClient is also used by send method, if we set it to nil here, the send may meet the nil pointer, so I did not set it to nil, what's your suggestion?

even though, the send will fail right? I think there might need a rwlock in send otherwise there is always a change that the two threads race.

I mean after backoff it will go to select in the next loop while client is not instantiated, and it will not have a chance to receive message from c.cloudEventsErrChan

yes, the send will be fail, we maybe need a lock here for the client

the c.cloudEventsErrChan will be only send by a running client, so if the client is retrying, the select will not be blocked the next retry

qiujian16 · 2023-09-22T01:18:09Z

cloudevents/generic/baseclient.go

+
+		for {
+			if cloudEventsClient == nil {
+				klog.V(4).Infof("reconnecting the cloudevents client")


one thing missing is: what if the client just send but not subscribe? Do we alway assume that the client will send and subscribe together?

hm... I'm not sure, maybe we cannot assume this, in our case, it seems that for our case the send and sub are always together, but as a lib, we don't stop to separate the send and subscribe, user can send in one process, and sub in other process, maybe we need to have one retry mechanism for the client

separate the reconnect part from the subscription, PTAL

qiujian16 · 2023-09-25T02:39:06Z

cloudevents/generic/baseclient.go

+
+type receiveFn func(ctx context.Context, evt cloudevents.Event)
+
+type baseClient struct {


should we have a separated Connect(ctx context.Context) in baseClient? so we do not start goroutine during new?

use connect method instead of new method

Signed-off-by: Wei Liu <[email protected]>

qiujian16

/approve

openshift-ci · 2023-09-25T08:47:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, skeeey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

qiujian16 · 2023-09-25T09:44:23Z

Maybe @yanmxa or @morvencao could also take a look

morvencao · 2023-09-25T13:43:40Z

cloudevents/generic/baseclient.go

+					// make sure the current client is the newest
+					c.RLock()
+					cloudEventsClient = c.cloudEventsClient
+					c.RUnlock()


do we still need to lock it here? since it is locked at the beginning of the subscribe function

yes, we need it, the beginning lock is for the subscribe function (it ensure there is only one subscription go routine running), this lock is in a independent go routine (the subscribe lock will be released after this go routine starts)

emm, right, thanks for the explanation.

morvencao · 2023-09-26T02:25:26Z

looks good to me, leave this to @yanmxa

yanmxa · 2023-09-27T03:58:38Z

Reference: cloudevents/sdk-go#940

/LGTM

openshift-ci bot requested review from deads2k and mdelder September 15, 2023 01:36

openshift-ci bot assigned qiujian16 Sep 15, 2023

openshift-ci bot added the do-not-merge/hold label Sep 15, 2023

skeeey changed the title ~~✨ expose the network error hook~~ WIP ✨ expose the network error hook Sep 18, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Sep 18, 2023

skeeey force-pushed the cloudevents-connecterr branch from 344c2bf to 89b4d02 Compare September 19, 2023 10:05

skeeey changed the title ~~WIP ✨ expose the network error hook~~ WIP ✨ retry connecting the cloudevents client Sep 19, 2023

skeeey force-pushed the cloudevents-connecterr branch 2 times, most recently from 2ac7fb0 to f27e8ba Compare September 20, 2023 03:09

skeeey changed the title ~~WIP ✨ retry connecting the cloudevents client~~ ✨ retry connecting the cloudevents client Sep 20, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 20, 2023

openshift-ci bot removed the do-not-merge/hold label Sep 20, 2023

qiujian16 reviewed Sep 20, 2023

View reviewed changes

skeeey force-pushed the cloudevents-connecterr branch 2 times, most recently from b58b1c2 to d9de5d7 Compare September 21, 2023 07:53

qiujian16 reviewed Sep 22, 2023

View reviewed changes

skeeey force-pushed the cloudevents-connecterr branch 3 times, most recently from 264e03f to 4c4356b Compare September 22, 2023 09:27

qiujian16 reviewed Sep 25, 2023

View reviewed changes

reconnecting the cloudevents client

d80ec98

Signed-off-by: Wei Liu <[email protected]>

skeeey force-pushed the cloudevents-connecterr branch from 4c4356b to d80ec98 Compare September 25, 2023 05:12

qiujian16 reviewed Sep 25, 2023

View reviewed changes

openshift-ci bot added the approved label Sep 25, 2023

morvencao reviewed Sep 25, 2023

View reviewed changes

openshift-ci bot assigned yanmxa Sep 27, 2023

openshift-ci bot added the lgtm label Sep 27, 2023

openshift-merge-robot merged commit c2b66fa into open-cluster-management-io:main Sep 27, 2023
14 checks passed

skeeey deleted the cloudevents-connecterr branch September 27, 2023 04:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ retry connecting the cloudevents client #280

✨ retry connecting the cloudevents client #280

skeeey commented Sep 15, 2023 •

edited

Loading

skeeey commented Sep 15, 2023

skeeey commented Sep 15, 2023

skeeey commented Sep 20, 2023

qiujian16 Sep 20, 2023

skeeey Sep 20, 2023

qiujian16 Sep 20, 2023

skeeey Sep 20, 2023

qiujian16 Sep 21, 2023

qiujian16 Sep 21, 2023

skeeey Sep 21, 2023

qiujian16 Sep 22, 2023

skeeey Sep 22, 2023

skeeey Sep 22, 2023

qiujian16 Sep 25, 2023

skeeey Sep 25, 2023

qiujian16 left a comment

openshift-ci bot commented Sep 25, 2023

qiujian16 commented Sep 25, 2023

morvencao Sep 25, 2023

skeeey Sep 26, 2023 •

edited

Loading

morvencao Sep 26, 2023

morvencao commented Sep 26, 2023

yanmxa commented Sep 27, 2023


		type receiveFn func(ctx context.Context, evt cloudevents.Event)

		type baseClient struct {

✨ retry connecting the cloudevents client #280

✨ retry connecting the cloudevents client #280

Conversation

skeeey commented Sep 15, 2023 • edited Loading

Summary

skeeey commented Sep 15, 2023

skeeey commented Sep 15, 2023

skeeey commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiujian16 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 25, 2023

qiujian16 commented Sep 25, 2023

Choose a reason for hiding this comment

skeeey Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morvencao commented Sep 26, 2023

yanmxa commented Sep 27, 2023

skeeey commented Sep 15, 2023 •

edited

Loading

skeeey Sep 26, 2023 •

edited

Loading